Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Add Model.WorldPivotData custom getter/setter #393

Merged
merged 2 commits into from
Feb 20, 2024
Merged

Add Model.WorldPivotData custom getter/setter #393

merged 2 commits into from
Feb 20, 2024

Conversation

kennethloeffler
Copy link
Member

This PR closes #391 by adding a custom getter/setter for Model.WorldPivotData.

I'm a little shaky about what ought to happen when the setter receives a nil. It likely means that the model's PrimaryPart was set to nil elsewhere... I think the safest option is probably to do nothing?

Dekkonot
Dekkonot approved these changes Feb 20, 2024
View reviewed changes
Copy link
Member

@Dekkonot Dekkonot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure about the PrimaryPart thing. GetPivot works just fine without it, so requiring a PrimaryPart for it feels odd!

On the other hand, I get it and think us being conservative here is probably fine. Roblox doesn't necessarily have a way to convery "optional cframe" from within their engine, so it working regardless may just be an artifact of that.

Going to hesitantly approve this because it works and I understand the reasoning. If it ends up being a mistake, it's not like it's a huge ordeal to correct.

@Dekkonot Dekkonot merged commit 3f3d64a into rojo-rbx:master Feb 20, 2024
2 checks passed
@kennethloeffler
Copy link
Member Author

So, in my dazed confusion I did mess this up a little bit.

Model.WorldPivotData is only unoccupied for a newly created Model. If I set the pivot whatsoever, the property remains occupied regardless of if the model has a PrimaryPart or not. With this in mind, the if instance.PrimaryPart == nil then bit is crap and should be removed.

This also means that when we want the property to be unoccupied (i.e. when the setter receives a nil), we may have to create a new Model instance. I'm not sure if this level of accuracy is necessary, but we should keep it in mind

@kennethloeffler kennethloeffler deleted the world-pivot-data-custom-getter-setter branch February 20, 2024 17:38
@kennethloeffler kennethloeffler mentioned this pull request Feb 20, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Dekkonot Dekkonot Dekkonot approved these changes

Assignees
No one assigned
Labels
module: rbx_dom_lua module: reflection database
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Model.WorldPivotData needs a custom property setter/getter
2 participants
@kennethloeffler @Dekkonot